Skip to content

[COD-3196] fix(callgrind): represent os threads under --separate-threads=yes - #24

Merged
GuillaumeLagrange merged 2 commits into
masterfrom
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os
Jul 30, 2026
Merged

[COD-3196] fix(callgrind): represent os threads under --separate-threads=yes#24
GuillaumeLagrange merged 2 commits into
masterfrom
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown
Contributor

Make callgrind per-thread dumps (--separate-threads=yes) behave like OS
threads: a thread spawns, runs, ends, and its costs stay attributed to it —
under the part being dumped, kept separate from other threads that reused its
slot, and tagged with its name.

Why it misbehaved

Two independent causes:

  1. Slot reuse. The valgrind core recycles ThreadId slots on exit.
    Callgrind keyed all per-thread state — and BBCC identity — on that slot with
    no thread-exit hook, so a new OS thread landing on a recycled slot silently
    appended its costs to the dead thread's containers.
  2. Mid-run dumps only flushed the caller. CALLGRIND_DUMP_STATS dumped only
    the current thread; since dumping is destructive, every other thread's whole
    history was deferred to the termination dump, showing up once under the final
    part.

What changed

  • Each thread_info gets a monotonic serial, independent of the recycled
    slot; used for the thread: header and -NN file suffix.
  • BBCC identity re-keyed on the serial (was the raw tid). This is the real
    fix for slot reuse — the per-BB LRU cache and hash lookup compared the tid, so
    a reused slot otherwise re-merged or tripped the clone_bbcc assertion.
  • A pre_thread_ll_exit hook unwinds the exiting thread, snapshots its name,
    and retires it (freed once flushed). Gated on separate_threads — the =no
    path is byte-for-byte untouched.
  • Every thread is flushed at each part dump (live + retired), so each
    thread's delta lands under the part being dumped. Zero-delta threads are
    skipped uniformly (incl. termination), which cleanly absorbs a thread that
    lived entirely inside an instrumentation-off window.
  • CALLGRIND_ZERO_STATS zeros all threads under separate_threads.
  • New VG_(get_thread_name) tool API surfaces the core's thread name, emitted
    as a desc: Thread name: line for the backend (COD-3197) to parse.

The retire path is safe on an already-unwound/empty state, so it survives the
STOP/START_INSTRUMENTATION the codspeed integrations wrap benchmarks in.

Review notes

  • callgrind/threads.c (retire + retired-list iteration) and callgrind/dump.c
    (per-part flush, retired-thread install, zero-delta skip) are the core of the
    change; bbcc.c is the serial re-keying.
  • The main thread is itself retired on its own exit before finalisation, so the
    termination dump also iterates the retired list.

Validation

Two new regression tests: thread-serial (sequential named workers with a dump
between them → two distinct serials, name emitted, costs under the dumped part)
and thread-instr (instrumentation toggling + an off-window thread → no crash,
no spurious section). Full callgrind make check (25 tests, incl. the existing
--separate-threads=yes threads/threads-use) and cachegrind suite (17,
guards the shared core change) pass. Manually re-ran the COD-3188 repros
(sequential, parallel, dump-while-running) — topology as expected.

Draft: opening for early review; will un-draft once the backend side (COD-3197)
is ready to consume the format.

Refs COD-3196